Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KVStore: Record table-wise memory usage and warn when it goes beyond the threshold #9835

Merged
merged 56 commits into from
Feb 17, 2025

Conversation

CalvinNeo
Copy link
Member

@CalvinNeo CalvinNeo commented Jan 27, 2025

What problem does this PR solve?

Issue Number: ref #9722

Problem Summary:

  • Introduce a struct RegionTableSize which is shared by a Table and all its related Regions. Once registered, a region will update its memory change to this struct, so the table would learn.
  • Introduce maybeWarnMemoryLimitByTable and resetWarnMemoryLimitByTable, to log when the memory of a table goes beyond threshold.

Both of the systems works under the path that:

  • When a region is created, either for prehandling or inserting, it would register it to RegionTable, and get its shared RegionTableSize.
  • Ever since, the insert/delete from the Region would also reflect on this shared RegionTableSize.
  • After an insertion, if the memory consumed by a table associated with the given region is reported to go beyond the threshold propotion, and the total memory used is also beyond the given memory limit(which is a very corner case), then we will issue a log.
  • Afterwards, if the memory usage somehow goes upper, we don't print the log again for the same table.
  • If the memory goes down by committing txns, or applying snapshots, we will reset the warning. So if the table still consumes much memory, a new log will be printed.
  • If the region is splitted or merged, the table's memory usage is not affected. If the region is destroyed, the table's memory would be decresed.

Also:

  • Remove some useless structures in RegionTable.
  • Make safe ts related codes into SafeTsMgr.
  • Rename some methods in Region to reflect what they actually do.

What is changed and how it works?

Record table-wise memory usage and warn when it goes beyond threshold

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Add logging when the uncommitted in-memory data of a table reach a threshold

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 27, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2025
CalvinNeo and others added 5 commits February 5, 2025 17:55
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@CalvinNeo CalvinNeo changed the title dnm Record table-wise memory usage Record table-wise memory usage Feb 7, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@CalvinNeo CalvinNeo changed the title Record table-wise memory usage Record table-wise memory usage and warn when it goes beyond threshold Feb 8, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/KVStore/Region.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/KVStore/Region.cpp Outdated Show resolved Hide resolved
dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 17, 2025
CalvinNeo and others added 6 commits February 17, 2025 14:13
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

We need to investigate why "It shows a byte cost in KVStore will result in 2 bytes cost in memory usage." later

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 17, 2025
@@ -107,6 +108,7 @@
#include <fiu.h>
#endif

std::atomic<UInt64> tranquil_time_rss;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO could be moved into KVStore to avoid this atomic. Not necessarily in this PR.

Copy link
Contributor

ti-chi-bot bot commented Feb 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, JinheLin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JaySon-Huang,JinheLin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 17, 2025
Copy link
Contributor

ti-chi-bot bot commented Feb 17, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-02-17 06:23:41.122012474 +0000 UTC m=+856063.518234536: ☑️ agreed by JaySon-Huang.
  • 2025-02-17 07:33:12.413806105 +0000 UTC m=+860234.810028166: ☑️ agreed by JinheLin.

ti-chi-bot bot and others added 4 commits February 17, 2025 07:36
Co-authored-by: jinhelin <linjinhe33@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@CalvinNeo
Copy link
Member Author

/retest

@JaySon-Huang
Copy link
Contributor

/test pull-unit-test

@ti-chi-bot ti-chi-bot bot merged commit ef110bf into pingcap:master Feb 17, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants